Skip to content

fix: rs274 nullptr will collapse the milltask when hal para is wrong#3465

Merged
andypugh merged 3 commits intoLinuxCNC:masterfrom
fghnntp:fix-bug-rs274-collapse
Jun 17, 2025
Merged

fix: rs274 nullptr will collapse the milltask when hal para is wrong#3465
andypugh merged 3 commits intoLinuxCNC:masterfrom
fghnntp:fix-bug-rs274-collapse

Conversation

@fghnntp
Copy link
Copy Markdown
Contributor

@fghnntp fghnntp commented Jun 7, 2025

rs274 nullptr will collapse the milltask when hal para is wrong.

@andypugh
Copy link
Copy Markdown
Collaborator

This looks reasonable on inspection. Can you describe how to demonstrate the problem that this fixes?

@fghnntp
Copy link
Copy Markdown
Contributor Author

fghnntp commented Jun 17, 2025

When using the rs274 model directly, you might write code like this:
InterBase *b = makeInterp();
b->init();
In the init function, SETG54_OFFSET() and maybe_new_line() is called. If the static pinterp is NULL at this point, it will suddenly cause the process to collapse without any error message or warning.
This makes the rs274 module not robust, especially when used independently.

@BsAtHome
Copy link
Copy Markdown
Contributor

Apparently, my review comments were not seen, again, ...

- static void maybe_new_line(int sequence_number=pinterp->sequence_number());
+ static void maybe_new_line(int sequence_number=0);
  static void maybe_new_line(int sequence_number) {
      if(!pinterp) return;
      if(interp_error) return;
+     sequence_number=pinterp->sequence_number()

Your fix would alter the code's operation. The change will assign sequence_number unconditionally. There are also calls where the default argument is overridden, which you now unconditionally replaced (see lines 335, 355, 372, 617 and 629).

@fghnntp fghnntp force-pushed the fix-bug-rs274-collapse branch from d30ea71 to 1dbe608 Compare June 17, 2025 09:21
@fghnntp
Copy link
Copy Markdown
Contributor Author

fghnntp commented Jun 17, 2025

Thank you for your feedback and clarification.
I have updated the code according to your suggestion:

  • I removed the default argument that depended on a runtime variable.
  • Now, there are two overloaded functions: one with no argument (which uses the current sequence_number from pinterp), and one that accepts a specified sequence_number.
  • This avoids unconditionally overwriting the parameter and keeps the logic clear and robust.

Sorry for the earlier mistake, and thank you for pointing it out.

@BsAtHome
Copy link
Copy Markdown
Contributor

Why a full duplication? Can't you just thunk the arg-less version like in:

static void maybe_new_line(int sequence_number);
static void maybe_new_line();

static void maybe_new_line(int sequence_number)
{
...
}

static void maybe_new_line()
{
    if(!pinterp) return;
    maybe_new_line(pinterp->sequence_number());
}

@fghnntp
Copy link
Copy Markdown
Contributor Author

fghnntp commented Jun 17, 2025

Thanks for the suggestion! I've updated the code to delegate the argument-less version to the one with sequence_number, removing the duplication.

@BsAtHome
Copy link
Copy Markdown
Contributor

Now it should be fine.

@andypugh andypugh merged commit 7218711 into LinuxCNC:master Jun 17, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants